Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EFM] Update DKG engine to submit a valid DKG index map #6490

Merged

Conversation

durkmurder
Copy link
Member

@durkmurder durkmurder commented Sep 23, 2024

#6319

Context

This PR modifies the existing logic to construct and submit DKG Index Map for each participant. Updated DKG reactor engine, client, broker. Since smart contracts were changed as well, this PR updates how we interact with he on chain logic using new API.
Specifically:

  • successful DKG and failure are submitted using different endpoints
  • added support for updated method signature of successful DKG API call

Updated test suites and DKG emulator integration tests.

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2024

Codecov Report

Attention: Patch coverage is 31.42857% with 120 lines in your changes missing coverage. Please review.

Project coverage is 41.72%. Comparing base (57a10de) to head (4c2d11a).

Files with missing lines Patch % Lines
module/dkg/client.go 0.00% 64 Missing ⚠️
module/dkg/broker.go 20.00% 20 Missing ⚠️
module/mock/dkg_contract_client.go 0.00% 17 Missing ⚠️
module/dkg/controller_factory.go 0.00% 9 Missing ⚠️
integration/dkg/dkg_client_wrapper.go 28.57% 5 Missing ⚠️
integration/dkg/dkg_whiteboard_client.go 71.42% 2 Missing ⚠️
utils/unittest/execution_state.go 0.00% 2 Missing ⚠️
integration/dkg/dkg_emulator_suite.go 97.72% 1 Missing ⚠️
Additional details and impacted files
@@                   Coverage Diff                    @@
##           feature/efm-recovery    #6490      +/-   ##
========================================================
+ Coverage                 41.43%   41.72%   +0.28%     
========================================================
  Files                      2029     2028       -1     
  Lines                    144665   180149   +35484     
========================================================
+ Hits                      59941    75166   +15225     
- Misses                    78541    98797   +20256     
- Partials                   6183     6186       +3     
Flag Coverage Δ
unittests 41.72% <31.42%> (+0.28%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@tarakby tarakby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen the core-contract update, but based on this PR, there may be a conceptual point about the index map that we need to align about. I left a comment below:

module/dkg.go Outdated
// SubmitResult must be called strictly after the final phase has ended.
SubmitResult(crypto.PublicKey, []crypto.PublicKey) error
// SubmitResult must be called strictly after the final DKG phase has ended on happy path.
SubmitResult(crypto.PublicKey, []crypto.PublicKey, flow.DKGIndexMap) error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason of including the index map as part of the results?
There may be a software design reason I don't know, but from a conceptual point of view, the index map does not result from a participant executing the protocol (like the keys), and there is no concept of "voting" for the right index map. The index map is a public info that is fixed before the protocol starts, it is the same from the point of view of all nodes.
The DKG contract should already know the index map once the list of participating nodes is finalized (before DKG starts), and not rely on the nodes to vote for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your point about it not being necessary. The smart contract could construct the index map from the participant list, with implementation changes. It does not directly have this information, because nowhere in the smart contracts do we have the notion of canonical ordering of identities: we treat them as sets. There wasn't a motivation to change this - hence the inclusion of the index map in the result.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the choice and had a look at onflow/flow-core-contracts#441 to understand the contract change. It would be good to clarify the implementation choice in the contract PR, since it may be confusing to see the indices as part of the result, and being voted for.

I also understand that the index map will only be available to the contract once enough honest participant voted for it as part of their submission. Is there an edge case of failure where the contract needs to access that map while not enough honest submissions have been gathered yet? just making sure there isn't such a case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an edge case of failure where the contract needs to access that map while not enough honest submissions have been gathered yet?

We don't have such an edge case with the current implementation. The DKG contract in general has very little exposure to details of the DKG protocol. For example, whiteboard messages are opaque to the contract, and the contract's only interaction with result submissions (and by extension the index map) is equality comparison to evaluate whether two result submissions are the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant is: the purpose of making the contract aware of the index map is to persist the index map, so that some nodes can query it if they don't have that context locally. But what if some nodes want to query that index map, and the contract doesn't know the map yet (because for some reason it didn't collect enough votes). That's the edge case I'm wondering about.
The information about the index map exists as soon as the beacon committee is finalized, but the contract only knows that information once DKG is over and it collected enough votes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the purpose of making the contract aware of the index map is to persist the index map, so that some nodes can query it if they don't have that context locally

I would say that the purpose of making the contract aware of the index map is so that end result of the DKG process is informationally complete. In particular, it is useful for the EpochCommit to contain a full description of the DKG result (including the index mapping).

Node interactions with the DKG smart contract are limited to:

  • read whiteboard messages
  • write whiteboard messages
  • write final result submissions

Nodes do not query the smart contract for the index map if they don't have that context locally. They only receive the DKG result via EpochCommit events, which are only produced after a DKG instance successfully completes.

Copy link
Member

@AlexHentschel AlexHentschel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice clean code, great tests and well documented. Thanks.

@durkmurder durkmurder merged commit 4c5b4ba into feature/efm-recovery Oct 18, 2024
55 checks passed
@durkmurder durkmurder deleted the yurii/6319-dkg-reactor-pushes-index-map branch October 18, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants